initial SOF hdac serials. by keyonjie · Pull Request #47 · thesofproject/linux

keyonjie · 2018-07-20T12:38:28Z

No description provided.

plbossart · 2018-07-20T14:31:03Z

this looks odd. why is this needed in the boards Kconfig?
Looks further, this makes no sense, in the same patch you have

select SND_SOC_SOF_HDA if SND_SOC_SOF_HDA_COMMON

+config SND_SOC_SOF_HDA

tristate

depends on SND_SOC_SOF_HDA_COMMON

so you've expressed a dependency twice for no good reason

um, this do need rewrite, I will do that. But before that, can you explain to me that is selecting 2 machine driver(e.g. BXT_TDF8532_MACH and SKL_HDA_DSP_GENERIC_MACH) together is our intention?

plbossart

This is a big series and we should probably try to think about the code structure to make it less invasive. See comments below

plbossart · 2018-07-20T14:32:22Z

do we really want hda_bus at the snd_sof_dev level? This is a bit Intel-centric, maybe it should be hidden in HDA specific structures, no?

today I will work on this, Liam suggest me to unify it with sof_intel_hda_dev(delete sof_intel_hda_dev if possible), Mengdong give a good suggestion that using private struct for it then other arch(e.g. ARM, non-HDA stuff) can reuse it.

plbossart · 2018-07-20T14:34:37Z

I thought we needed different ring buffers for CORB and RIRB, similar to capture and playback?

they are allocated together(4KB) and corb/rirb are initialized to use different regions( pls refer to snd_hdac_bus_init_cmd_io()):
bus->corb.addr = bus->rb.addr;
bus->rirb.addr = bus->rb.addr + 2048;

plbossart · 2018-07-20T14:37:43Z

You probably want to rename this Kconfig to CONFIG_SND_SOC_SOC_HDA_LINK ?
HDA can mean controller, bus or codec. Here you handle the two latter topics.

good suggestion, will change it.

plbossart · 2018-07-20T14:38:29Z

avoid copy/pastes from GPL comments

will remove/rewrite them, thanks.

plbossart · 2018-07-20T14:40:10Z

that looks wrong. We get the PCM period updates from the existing SOF code, why do we need to do this based on link information?

plbossart · 2018-07-20T14:44:15Z

could we have an .ops here and select which mode we want? It'd allow all the HDA link part to go in a different file. Ifdefs are hard to handle at some point, it's difficult to read.

OK, will change it, thanks.

plbossart · 2018-07-20T14:45:56Z

the defines are probably wrong, if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDA) is false, the ext_ops is not initialized. Is this is intentional you've got to comment it.

um, this comes from cAVS, no chance to debug it yet, if soc_hdac_hda unselected, I think it may pass ext_ops with NULL and no ops(e.g. attach/detach) will be processed?

plbossart · 2018-07-20T14:47:35Z

the part below is super hard to review.

um, Liam helps to refine and consolidate them in his consequent commit.

Differentiation was moved to separate new functions to avoid lots of conditional compilation logic in a single function.

plbossart · 2018-07-20T14:50:16Z

I suggest we don't make the same mistake as cAVS. the entire code for HDMI is common between platforms, so let's do it right from day 1, by adding the links from a common code instead of copy/pasting.

yes, we have problem here. So our intention is to add skl_hda_be_dai_links[] from skl_hda_dsp_common.c together with broxton_tdf8532_dais[] at broxton_tdf8532_audio_probe()?

keyonjie · 2018-07-23T10:48:27Z

updates on 0723:

refine Kconfig.
fix hdmi codec probing failed issue.

keyonjie · 2018-07-23T12:16:31Z

@lgirdwood @mengdonglin @plbossart please be noticed that it is updated.

keyonjie · 2018-07-24T12:21:43Z

@lgirdwood @mengdonglin @plbossart
new update today: we have finished to replace sof_intel_hda_stream with hdac_stream, and fix NULL pointer issue, now PCM created and playback works for I2S BE on gpmrb.

keyonjie · 2018-07-25T08:57:20Z

update on 0725:
We use CONFIG_SND_SOC_SOF_HDA to enable HDA links on SOF, refine
SOF HDA code to split them and make both select/unselect the config
CONFIG_SND_SOC_SOF_HDA works.

lgirdwood · 2018-07-26T10:25:58Z

+
+#include <sound/hdaudio.h>
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)


Isn't this implied by Kconfig option

are you meaning that IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) is true always?
the answer is no for that, it is tristate, we can choose them or not in menuconfig.

lgirdwood · 2018-07-26T10:27:04Z

+
+#endif
+
+static void sof_hda_reg_writel(u32 value, u32 __iomem *addr)


Any need to wrap these ? cant the ops point to writew() etc directly ?

just went through them again, no need to wrap them, let me change them, thanks.

sorry, double checked this, we can't point to writew() etc directly, as they are actually inline functions and will be expanded(not functions then), so we still need to wrap them.

lgirdwood · 2018-07-26T10:28:12Z

@@ -1,9 +1,9 @@
-// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)


Don't change the license.

Will modify.

lgirdwood · 2018-07-26T10:29:23Z

-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
-
-#if 0
-static int sof_hda_acquire_irq(struct hda_bus *hbus, int do_disconnect)


Is thsi MSI irq working ?

MSI doesn't work for HDA in my testing, so I set it to legacy mode temporary, will change it to MSI once it is verified works.

lgirdwood · 2018-07-26T10:30:39Z

 	int irq;
 };

+#define SOF_STREAM_SD_OFFSET(s) (0x20 * ((s)->index) + SOF_HDA_ADSP_LOADER_BASE)


where does 0x20 come from ?

we should change it to an macro, it is size of all registers belong to a separated stream.

lgirdwood · 2018-07-26T10:32:50Z

 		}
 	}

+


Do we need period_elapsed() ?

IMO, no, we do that in IPC interrupt handler.

lgirdwood · 2018-07-26T10:33:08Z

-	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev, 8,
-				  &hdev->posbuffer);
+	/* TODO: check postion buffer update */
+	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev, 8 * num_total,


Where does 8 come from ?

8 Bytes is the size of DMA posistion for each stream defined in HDA spec, let me change it to macro also.

lgirdwood · 2018-07-26T10:35:24Z

+	 *       support msi multiple vectors
+	 */
+//	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_MSI);
+	/* todo: MSI mode doesn't work for HDMI yet, debug it later */


Is this still true ? If so, temp workaround is to use LEGACY IRQ for HDAC mode (based on Kconfig) otherwise MSI.

lgirdwood · 2018-07-26T10:36:48Z

Differentiation was moved to separate new functions to avoid lots of conditional compilation logic in a single function.

lgirdwood · 2018-07-26T10:37:30Z


 config SND_SOC_SOF_HDA
-	tristate
+	tristate "SOF support for HDA Links(HDA/HDMI)"


Does this have a help section (check patch complains without one).

let me add it.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

We need register/enable irq handler before init_caps, where we are doing codec probing for HDMI/HDA, otherwise the probing may fail. We also need to use PCI_IRQ_LEGACY mode at the moment, as PCI_IRQ_MSI doesn't work for HDMI yet, may need more debug to fix it. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

This patch doesn't replace struct sof_intel_hda_dev with hda_bus, but embed hda_bus in sof_intel_hda_dev. It's becasue there are a few memebers of sof_intel_hda_dev are not in hda_bus. Host streams are managed by hdac_bus's stream_list now. Compiling okay. But not test yet. May have conflict with Keyon's PR today. I'll rebase and merge it to Keyon's PR later. Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

In previous patch, hdac_bus is used to manage the host streams, but the stream structure is still struct sof_intel_hda_stream. This patch replaces struct sof_intel_hda_stream with hdac_ext_stream, but the stream management is still using BSD implementation of SOF, not the GPL implemenation of HDA core. Please note that - hdac_bus maintains a stream list of hdac_stream, which is embeded in struct hdac_ext_stream. - pcm runtime's private data points to hdac_stream, not hdac_ext_stream. Compiling okay. Not tested yet. Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

…issue. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

1. add hda-bus to handle hda bus initialization, for both with/without HDA links support. 2. split hda-codec out from hda-common, and compile it when SOF_HDA is selected. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

We uses CONFIG_SND_SOC_SOF_HDA to enable HDA links on SOF, here refine SOF HDA code to split them and make both select/unselect the config CONFIG_SND_SOC_SOF_HDA works. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie · 2018-07-27T11:26:33Z

all are addressed. @lgirdwood

plbossart reviewed Jul 20, 2018

View reviewed changes

keyonjie force-pushed the sof-hda branch from df12103 to 1f9deeb Compare July 23, 2018 10:38

keyonjie force-pushed the sof-hda branch from 1f9deeb to 0af1520 Compare July 23, 2018 13:03

lgirdwood requested changes Jul 26, 2018

View reviewed changes

keyonjie added 2 commits July 27, 2018 18:08

ASoC: SOF: refine Kconfig for SOF HDA.

f0e1f9b

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

keyonjie force-pushed the sof-hda branch 2 times, most recently from f22e0d3 to 410c8ea Compare July 27, 2018 10:34

mengdonglin added 2 commits July 27, 2018 19:20

keyonjie force-pushed the sof-hda branch from 410c8ea to bd37bda Compare July 27, 2018 11:23

keyonjie added 3 commits July 27, 2018 19:23

ASoC: SOF: hda: initial bus pointer for stream to fix a NULL pointer …

7cc5deb

…issue. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

ASoC: SOF: hda: refine makefile for with/without HDA support

3ea24e9

1. add hda-bus to handle hda bus initialization, for both with/without HDA links support. 2. split hda-codec out from hda-common, and compile it when SOF_HDA is selected. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

ASoC: SOF: hda: refine to support both with/without HDA links.

bd37bda

We uses CONFIG_SND_SOC_SOF_HDA to enable HDA links on SOF, here refine SOF HDA code to split them and make both select/unselect the config CONFIG_SND_SOC_SOF_HDA works. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>

lgirdwood merged commit 2f7d85a into thesofproject:topic/hdac Jul 30, 2018

ranj063 mentioned this pull request Jan 9, 2019

[Stress test]: Kernel panic after unloading and reloading SOF modules 18 times on APL with PCM512x. #490

Closed


		#include <sound/hdaudio.h>

		#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)


		#endif

		static void sof_hda_reg_writel(u32 value, u32 __iomem *addr)

		@@ -1,9 +1,9 @@
		// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

Uh oh!

Conversation

keyonjie commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented Jul 23, 2018

Uh oh!

keyonjie commented Jul 23, 2018

Uh oh!

keyonjie commented Jul 24, 2018

Uh oh!

keyonjie commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment